Support generating Python SDK with Pyodide #4784
Conversation
microsoft-github-policy-service
bot
added
the
emitter:client:python
|
No changes needing a change description found. |
|
You can try these changes here
|
YalinLi0312
requested review from
iscai-msft,
tadelesh,
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
| try { | ||
| Write-Host 'Checking for differences in generated code...' | ||
| & "$packageRoot/eng/scripts/Check-GitChanges.ps1" | ||
| Write-Host 'Done. No code generation differences detected.' |
There was a problem hiding this comment.
have you made sure that this will catch differences in the generated code? You can try passing in an extra flag or something when generating with pyodide to make sure that any changes are caught
| @@ -242,6 +245,9 @@ function addOptions( | |||
| const emitterConfigs: EmitterConfig[] = []; | |||
| for (const config of getEmitterOption(spec)) { | |||
| const options: Record<string, string> = { ...config }; | |||
| if (flags.pyodide) { | |||
There was a problem hiding this comment.
nit: it might be better to keep the flags having the same name. I know we might not do this everywhere, but might as well when adding new flags
| @@ -30,12 +30,12 @@ | |||
| }, | |||
| "scripts": { | |||
| "clean": "rimraf ./dist ./temp ./emitter/temp ./generator/test/azure/generated ./generator/test/unbranded/generated ./venv", | |||
| "build": "tsc -p ./emitter/tsconfig.build.json", | |||
| "build": "tsc -p ./emitter/tsconfig.build.json && tsx ./eng/scripts/setup/run-python3.ts ./eng/scripts/setup/build_pygen_wheel.py && rimraf ./venv_build_wheel", | |||
There was a problem hiding this comment.
nit: i would follow laurent's comments and clean up the package.json scripts as well in this pr
| if (sdkContext.arm === true) { | ||
| commandArgs.push("--azure-arm=true"); | ||
| } | ||
| if (resolvedOptions.flavor === "azure") { |
There was a problem hiding this comment.
The logic about options (e.g. "azure-arm"/"flavor") between python mode and pyodide mode is same so it is better to handle them in one central place.
| Write-Host "Building project ..." | ||
| & npm run build | ||
|
|
||
| Write-Host "Regenerating project with Pyodide ..." |
There was a problem hiding this comment.
I think there is no need to add new step in eng to run test twice with python and pyodide mode to make the CI time double than now. We can run azure test with python mode and unbranded test with pyodide mode. Then you just need to add use-pyodide: true here
| resolvedOptions["package-pprint-name"] = `"${resolvedOptions["package-pprint-name"]}"`; | ||
| } | ||
| if (resolvedOptions["use-pyodide"]) { | ||
| const commandArgs: Record<string, string> = {}; |
There was a problem hiding this comment.
the command args should be same whatever use pyodide or not, it's better to consolidate the logic.
| async def main(): | ||
| import warnings | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore", SyntaxWarning) | ||
| from pygen import m2r, preprocess, codegen, black | ||
|
|
||
| m2r.M2R(output_folder=outputFolder, cadl_file=yamlRelativePath, **commandArgs).process() | ||
| preprocess.PreProcessPlugin(output_folder=outputFolder, cadl_file=yamlRelativePath, **commandArgs).process() | ||
| codegen.CodeGenerator(output_folder=outputFolder, cadl_file=yamlRelativePath, **commandArgs).process() | ||
| black.BlackScriptPlugin(output_folder=outputFolder, **commandArgs).process() | ||
|
|
||
| await main() |
There was a problem hiding this comment.
is pyodide able to run a py file? i just wondering if two paths could be consolidated. previously, we used py file. here, we use inline code.
|
|
||
| await main() | ||
| `; | ||
| await pyodide.runPythonAsync(python, { globals }); |
There was a problem hiding this comment.
how will pyodide deal with exception?
| await runPython3("./eng/scripts/setup/install.py"); | ||
| await runPython3("./eng/scripts/setup/prepare.py"); |
There was a problem hiding this comment.
i'm wondering how the exception be caught here?
No description provided.